-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make fn update() re-render the component by default #2786
Make fn update() re-render the component by default #2786
Conversation
Visit the preview URL for this PR (updated for commit 0f50ca9): https://yew-rs-api--pr2786-cecton-update-compon-cfkeqm2z.web.app (expires Wed, 27 Jul 2022 14:30:48 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 |
Size Comparison
✅ None of the examples has changed their size significantly. |
This PR is ready. |
To answer as to why I made update return false: It was like this in the original PR #1646 See: https://github.com/yewstack/yew/pull/1646/files#diff-f25ab267a13ad40d72e4e71be976e17a39cb8eb5afb632e047a00dc6a3e42f0bR19-R24 |
Thanks! I will investigate further |
@hamza1311 so I asked Justin by email (didn't want to bother him with notifications) and he said:
So I guess it's safe to merge 🙌 |
I suppose, a component that doesn't want to receive messages should use an empty message type, i.e. |
I never thought about that but you are correct. Sometimes, in Yew 0.18, I make an empty enum (which is equivalent to fn update(msg: Msg) -> bool {
match msg {}
} On the other hand, with component v2, we don't need to write a type for a component when we don't use messages so there is no need to be explicit about that. However, it's particularly handy to have the default message on
True... Minor detail though... imo it's not like you send messages by mistake. |
Just wondering, what can a refresh without updating any state do? You can't modify any component state from view because it takes The only place I can see it being useful are components which use interior mutability, but at that point, I imagine the component the component will be complex enough to have a message type I was thinking maybe we could default the message type to |
If your component's rendering depends on external variables, then refreshing is very useful. For example, when using a global state for your application you would want to refresh components independently.
It makes sense alright but it's unnecessary as nobody really cares what is the type of the message when the message is not used. I mean you can have Making the component refresh is a bit of a niche trick but it doesn't harm anyone: anybody who define their own message will override this function anyway.
Would that force yew to nightly? I would prefer that we keep Yew in stable unless there is a very important feature. In this case, using |
I've should've been been more clear. I agree with the change.
No, we have a feature flag that users can enable if they're on nightly. Line 100 in 4eda943
It really should be a compiler flag though: #2754 |
Thanks! I'll wait a bit to get a second approval to be safe |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is some potential source of confusion from re-renders of components if these "spurious" update messages ever get used in a library facility (think checks such as done React Strict Mode) but they would most likely be opt-in by default and I don't want to design around future possibilities anyway. Completely fine right now, for poking your own components.
Description
Make
Component
's methodfn update()
re-render the component by default. See #1961 (comment)Putting this to
true
is very handy because it allows a basic component that only needs to refresh with a callback to refresh without having to define thefn update()
method.Related to #1961
Checklist